fix(harness): exclude shadowed lower grep matches in OverlayFilesystem - #2450
fix(harness): exclude shadowed lower grep matches in OverlayFilesystem#2450Blue-Berrys wants to merge 1 commit into
Conversation
When an upper-layer file exists at the same path, lower-layer grep hits for that path must not be returned even if the upper file no longer matches. Closes agentscope-ai#2448.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Review — Approved ✅
Clean fix for the stale lower-layer grep match issue in OverlayFilesystem.
Code analysis:
- The shadowing logic correctly uses
upper.exists()to determine whether a lower-layer path is fully overridden, regardless of whether the upper file still matches the pattern. upperShadowedPathsset avoids redundantupper.exists()calls for duplicate paths in lower matches.- Regression tests cover both key scenarios: upper-edit-removes-match and upper-match-overrides-lower.
Minor note: For very large grep result sets, the per-path upper.exists() calls could add up. Consider batching if this becomes a bottleneck in practice — but not a blocker.
One nit: the upperShadowedPaths.contains(matchPath) check on line 217 is redundant after the block above already adds to the set and continues. Could simplify to just the upper.exists() check + add + continue pattern.
LGTM. Please sign the CLA when you get a chance.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
4a86955 to
ce8352f
Compare
AgentScopeJavaBot
added
bug
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
Fixes a shadowing bug in OverlayFilesystem.grep() where lower-layer grep matches would incorrectly leak through when the upper layer has a file at the same path but that file no longer contains the search pattern. Note: PR title says "fix tool schema validation edge cases" but diff is about OverlayFilesystem grep shadowing.
(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
Fixes a shadowing bug in OverlayFilesystem.grep() where lower-layer grep matches would incorrectly leak through when the upper layer has a file at the same path but that file no longer contains the search pattern. Note: PR title says "fix tool schema validation edge cases" but diff is about OverlayFilesystem grep shadowing.
(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
Fixes a shadowing bug in OverlayFilesystem.grep() where lower-layer grep matches would incorrectly leak through when the upper layer has a file at the same path but that file no longer contains the search pattern. Note: PR title says "fix tool schema validation edge cases" but diff is about OverlayFilesystem grep shadowing.
(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)
4 participants
Summary
OverlayFilesystem.grepso upper-layer files fully shadow lower-layer matches for the same path, even when the upper file no longer contains the pattern.Closes #2448
Description
OverlayFilesystemdocuments that upper entries override lower entries on path collision, butgreppreviously merged bypath:lineonly. When an upper-layer file existed at the same path but no longer matched, the lower-layer hit remained becauseupperResulthad no entry for that path.The fix checks
upper.exists(...)for each lower match path and skips shadowed lower matches before merging upper hits.Checklist
mvn spotless:applyOverlayFilesystemGrepTest)Testing
JAVA_HOME=/path/to/jdk21 mvn -pl agentscope-harness -am \ -Dspotless.check.skip=true \ -Dtest=OverlayFilesystemGrepTest \ -Dsurefire.failIfNoSpecifiedTests=false \ test